Skip to content

[V4] Update existing service tests to use async operations #4178

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
dscpinheiro merged 2 commits into from
Dec 3, 2025
Merged

[V4] Update existing service tests to use async operations #4178

dscpinheiro merged 2 commits into from
Dec 3, 2025

Conversation

@dscpinheiro
Copy link
Contributor

@dscpinheiro dscpinheiro commented Dec 1, 2025

Follow up for #4177, this PR updates the tests for services with hand-written code to use the Async operations. Most of the changes here are similar to Client.GetRestApi -> await Client.GetRestApiAsync but some important things to call out:

  • I did not include S3 or DDB since those are the majority of tests (and there's parallel work on those services). I'll address them in their own PR later
  • If the tests were not validating the custom code, I deleted the test project altogether. For example, the hand-written code for CloudFormation is about updating a stack (https://github.com/aws/aws-sdk-net/tree/main/sdk/src/Services/CloudFormation/Custom), but we were testing other operations such as ValidateTemplate instead (with a aws-vs-toolkit.s3.amazonaws.com URL...)
  • If the test was broken and already ignored, I deleted it as well. For example, Bedrock had a test for event streaming but a) the model identifier is not supported anymore, and b) newer models require a different syntax.
  • (This is the most controversial one) If the tests are not being run in our build system today, I got rid of them as well. There are some tests that need long-term credentials (i.e. an IAM user) and are marked with a specific category (which we ignore during releases). Note most of them aren't for custom code but I thought I might as well include them in the refactoring.

Testing

Dry-run: DRY_RUN-f2020ce8-4c7b-4f82-80aa-24b63d923a66

License

  • I confirm that this pull request can be released under the Apache 2 license

@dscpinheiro dscpinheiro added the v4 label Dec 2, 2025
Copilot finished reviewing on behalf of GarrettBeatty December 3, 2025 16:05
Copilot AI reviewed Dec 3, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR modernizes integration tests by converting synchronous operations to async/await patterns across multiple AWS service test suites. The changes align with the repository's goal of adopting modern .NET async patterns while also removing tests that don't validate custom hand-written code or that require special credentials (IAM users) not available in the CI/CD pipeline.

Key Changes:

  • Converted test methods from synchronous to async (void → async Task)
  • Updated all service client calls to use Async methods with await
  • Replaced Thread.Sleep() with Task.Delay()
  • Removed tests marked with [TestCategory("RequiresIAMUser")] that aren't run in the build system
  • Deleted entire test projects for services where tests didn't validate custom code (RDS, Lambda, KMS, IAM, ELB, etc.)

Reviewed changes

Copilot reviewed 53 out of 53 changed files in this pull request and generated no comments.

Show a summary per file
File Description
WAF.cs Converted to async, changed yield return pattern to list collection for async compatibility
SNS.cs Converted to async, improved cleanup with null-conditional operators, removed ignored subscription test
SQS.cs Converted to async, simplified queue operations, improved code formatting
S3Control/PublicAccessBlockTests.cs Converted to async with proper test initialization
S3/KMSTests.cs Minor change to use KMS client with region endpoint configuration
Route53.cs Converted to async, removed flaky delegation set test, improved VPC test cleanup
Polly tests Converted to async, removed test requiring IAM user credentials
IotData tests Converted to async, improved cleanup logic
GameLift tests Converted to async, changed yield return to list collection
EC2 tests Converted to async, removed empty test file, improved security group tests
DynamoDBStreams tests Converted to async with minor config improvements
CloudWatch tests Removed extensive alarm tests, kept compression test
SecurityToken, RDS, Lambda, KMS, IAM, ELB projects Deleted entirely along with config files
CognitoSync Moved cache test to unit tests, deleted integration test project
ECS/CredentialsTests Removed session credentials test

GarrettBeatty
GarrettBeatty reviewed Dec 3, 2025
View reviewed changes
sdk/test/Services/CognitoSync/UnitTests/Custom/CacheItemsTests.cs
[ClassCleanup]
public static void ClassCleanup()
{
BaseClean();
Copy link
Contributor

@GarrettBeatty GarrettBeatty Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was baseclean not doing anything?

Copy link
Contributor Author

@dscpinheiro dscpinheiro Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this was an integration test that was validating properties via reflection... https://github.com/aws/aws-sdk-net/blob/main/sdk/test/Services/CognitoSync/IntegrationTests/CognitoSync.cs

Since I moved it to an unit test instead there's no need for a TestBase anymore, but for reference it just disposes the service client:

aws-sdk-net/sdk/test/IntegrationTests/Tests/TestBase.cs

Lines 34 to 41 in d7d3513

public static void BaseClean()
{
if (client != null)
{
client.Dispose();
client = null;
}
}

ashishdhingra
ashishdhingra reviewed Dec 3, 2025
View reviewed changes
sdk/test/Services/S3/IntegrationTests/KMSTests.cs
{
var keyId = KeyManagementService.Client.CreateKey(new Amazon.KeyManagementService.Model.CreateKeyRequest
var kms = new AmazonKeyManagementServiceClient(Client.Config.RegionEndpoint);
var keyId = kms.CreateKey(new Amazon.KeyManagementService.Model.CreateKeyRequest
Copy link
Contributor

@ashishdhingra ashishdhingra Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be using async version?

Copy link
Contributor Author

@dscpinheiro dscpinheiro Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So do not ask me why, but this is a test in the S3 project that was using the client from KMS... I plan to make the same update I made here (sync -> async) for S3 and DDB in their own PRs since those are the largest services (and with the highest number of tests).

Copy link
Contributor

@ashishdhingra ashishdhingra Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

Base automatically changed from dspin/test-async-refactoring-pt1 to dspin/test-async-refactoring December 3, 2025 18:18
@dscpinheiro dscpinheiro merged commit 5a8c822 into dspin/test-async-refactoring Dec 3, 2025
7 of 8 checks passed
@dscpinheiro dscpinheiro deleted the dspin/test-async-refactoring-pt2 branch December 3, 2025 18:20
@dscpinheiro dscpinheiro mentioned this pull request Dec 4, 2025
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@GarrettBeatty GarrettBeatty GarrettBeatty approved these changes

@ashishdhingra ashishdhingra ashishdhingra approved these changes

Assignees

No one assigned

Labels

v4

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dscpinheiro @GarrettBeatty @ashishdhingra